Skip to content

🐛 fix: match selected security policies by exact path - #5199

Open
bilaldeveloper4312 wants to merge 2 commits into
ossf:mainfrom
bilaldeveloper4312:fix/security-policy-exact-path
Open

🐛 fix: match selected security policies by exact path#5199
bilaldeveloper4312 wants to merge 2 commits into
ossf:mainfrom
bilaldeveloper4312:fix/security-policy-exact-path

Conversation

@bilaldeveloper4312

Copy link
Copy Markdown

Fixes #5198\n\nSecurity-Policy first discovers an approved policy path, but its content pass previously allowed a basename fallback. An unrelated nested security.md could therefore be read instead of the selected policy.\n\nThis adds an ExactPath option to the shared matcher and uses it only for Security-Policy content reads, preserving existing basename fallback behavior for other callers.\n\nValidation:\n- go test ./checks/fileparser ./checks/raw (Go 1.26.1, GitHub Codespaces)

Signed-off-by: Muhammad Bilal Mukhtar <designbybilal9@gmail.com>
@bilaldeveloper4312
bilaldeveloper4312 requested a review from a team as a code owner August 26, 2026 06:23
@bilaldeveloper4312
bilaldeveloper4312 removed the request for review from a team August 26, 2026 06:23
@dosubot dosubot Bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Aug 26, 2026
@bilaldeveloper4312 bilaldeveloper4312 changed the title fix: match selected security policies by exact path 🐛 fix: match selected security policies by exact path Aug 27, 2026

@noqt noqt left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOQT independently reviewed current head f2848a6.

The change is narrowly scoped: ExactPath preserves the existing basename fallback by default and is enabled only when Security-Policy rereads a path it already selected. The two focused exact-path cases and TestSecurityPolicy pass with Go 1.25.6; go vet ./checks/fileparser ./checks/raw and git diff --check also pass. The wider affected-package run still hits existing Windows workflow-fixture failures, which I reproduced on unchanged base d1fab88 as well.

This fixes the reported misread: a selected root SECURITY.md no longer matches .review-pro/node/security.md by basename.

Approved. One non-blocking hardening suggestion: add a SecurityPolicy-level regression containing both the selected policy and a same-basename nested file, so the caller's ExactPath wiring is protected as well as the matcher itself.

Signed-off-by: Muhammad Bilal Mukhtar <designbybilal9@gmail.com>
@tufantunc

Copy link
Copy Markdown

I filed #5198, so a note from the reporting side in case it helps a reviewer.

The shape here is the right one. Gating the fallback per call rather than changing isMatchingPath for everyone was what the issue suggested, for the reason the diff's own comment gives: the basename fallback is presumably load-bearing for other callers, and Security-Policy is the one caller that has already resolved an exact path before it re-reads the file. ExactPath defaulting to false keeps that asymmetry explicit.

One detail worth knowing when you check the fix. The affected repo scores 3, not 0, and the difference is diagnostic rather than cosmetic. With the wrong file read, the three probes land like this:

probe outcome points
securityPolicyContainsLinks false — the nested file has no URLs 0 of 6
securityPolicyContainsVulnerabilityDisclosure false — no reDIG hits either 0 of 1
securityPolicyContainsText true 3 of 3

The third one passes because it tests policy.File.FileSize > linkedContentLen + (urls+emails)*2 rather than the collected hits, so a non-empty wrong file still satisfies it. That combination — text passing while links and disclosure fail — is the signature of reading the wrong file, and it is why the symptom is a stubborn 3 rather than a 0. Expect 3 → 10 on a fixed run, and if you see 0 something else is wrong.

On the fixture. I see the tests use .review-pro/node/security.md, which is the real path from the reporting repo rather than a synthetic one — good, because it is also the reason the bug is reachable at all: a dot-directory sorts before SECURITY.md, so it wins the first-match. Worth keeping that property in the fixture if it ever gets renamed.

tufantunc/ssh-mcp is public and reproduces this on every run, if a live case is more useful than a fixture at any point: root SECURITY.md (12 KB, links + email + disclosure timeline) and two unrelated .review-pro/*/security.md rubric files with no policy signals in them.

I could not build scorecard in my own environment to confirm the fix end-to-end against that repo — the module download times out here — so I am not claiming to have verified it, only that the analysis and the approach line up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S This PR changes 10-29 lines, ignoring generated files.

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

BUG: Security-Policy scores the contents of an unrelated security.md found elsewhere in the tree

3 participants